-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
util/stmtsummary: migrate test-infra to testify #26771
Conversation
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/cc @tisonkun |
@feitian124 thanks for your contribution @feitian124 ! I'll review this PR tomorrow. |
@tisonkun ok, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you separate statement_summary_test.go
changes into another PR? This PR is too large to review.
sorry, it is large and take me some time to finish... i did reorder my commit and pushed to a new branch https://github.com/feitian124/tidb/commits/issue-26420-new, commit f01914e8ecf743b3a22edf81a49ed25eb9ac6c7d (HEAD -> issue-26420-new, my/issue-26420-new, issue-26420-part1)
Author: feitian124 <feitian124@gmail.com>
Date: Tue Aug 3 13:32:11 2021 +0800
util/stmtsummary: migrate test-infra to testify, part 2
util/stmtsummary/evicted_test.go | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------
util/stmtsummary/main_test.go | 26 ++++++++++++++++++++++++++
util/stmtsummary/variables_test.go | 98 +++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
3 files changed, 151 insertions(+), 116 deletions(-)
commit 68a3829fefe77aa229af61963710366bfe452120
Author: feitian124 <feitian124@gmail.com>
Date: Tue Aug 3 13:32:00 2021 +0800
util/stmtsummary: migrate test-infra to testify, part 1
util/stmtsummary/statement_summary_test.go | 615 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------------------------------------------------
1 file changed, 311 insertions(+), 304 deletions(-) not sure if this help. |
@feitian124 you can take a look at how we do it for domain pkg #26306, or if you separate in multiple commits in one PR said this one, it is fine also. |
Here is an example how to break diff into meaningful commits that helps on reviewing. https://github.com/apache/flink/pull/9832/commits |
hi @tisonkun , i have separate them to 3 pr, hopefully can make your review easier. |
What problem does this PR solve?
Issue Number: close #26420
Release note